Repository navigation
Add unit tests for the ScaleInvariantMatrixMatch kernels - #1601
Merged
Merged
Conversation
…rixMatch.cpp It was never used: the file throws no exceptions and includes nothing that needs it.
Add a TestInfra for the four kernel functions (compute_scale and compute_error, plain and weighted). Each test generates a random width x height matrix pair (and weight matrix for the weighted variants), runs the Default path and the ISA path on identical input, and compares the results. The Default path is the source of truth, so each ISA registers the same (width, height, use_weights) combinations against Default. The sizes cover 1x1 up to 1000x1000, both divisible and undivisible by the vector lengths (4, 8, 16), including the short-row cases that exercise unaligned row prefixes and partial vector tails. The SSE4.1 path back-reads up to 3 floats before a row start, so the matrices are allocated with 16 floats of head padding to keep those reads in-bounds. Scales are not computed exactly by all paths (different addition orders), so results are compared with a tolerance: pass if within 4096 ULPs, or within 1e-4 of the magnitude of the accumulated sum (the absolute part covers near-zero results, where ULPs are far smaller than the accumulated rounding of a multi-million-term sum). The tests are registered per-ISA in each x86 Core file (SSE, AVX2, AVX512), guarded by the same PA_AutoDispatch and CPU_CAPABILITY_CURRENT checks the dispatchers use, so each architecture only runs the tests for the ISAs it can execute.
The plain (unweighted) partial-tail of SumATA2::accumulate() and of SumError::accumulate() called load2_partial_front() with the A and T pointers swapped, so the tail lanes fed T where A was expected: - compute_scale accumulated T*T instead of A*A into sum_A2 for the tail lanes, biasing the scale for any row with a non-zero tail (width not divisible by the vector length); - compute_error accumulated (scale*T - A)^2 instead of (scale*A - T)^2 for the tail lanes. The weighted variants and the unaligned-row prefixes use the correct argument order; only the two plain tails were swapped. The new unit tests (Default as source of truth) catch this: with random data the scale error is on the order of the tail's share of the sum, far outside the 4096-ULP / 1e-4-magnitude tolerance.
The weighted partial-tail loads in SumATA2 and SumError passed the row-start pointers (A, TW, W), which had been shifted back by the unaligned-row prefix and then advanced by a full vector, instead of the current pointers (ptrA, ptrT, ptrW). The tail therefore reloaded the first 'length' elements of the row instead of the last ones. Found by the new unit tests.
Each ISA kernel requires a minimum row width (4 for SSE4.1, 8 for AVX2, 16 for AVX512): below it, the partial-row loads span past the row end and can read past the end of the allocation. The test registration now skips sizes below the ISA's minimum width. The test matrices also get 15 floats of tail padding after the last row: the SSE4.1 partial-tail load (PSHUFB-based) reads a full 16-byte word that extends up to VL - 1 floats past the row end, which would otherwise run past the allocation on the last row.
The diagnostics isolated the root cause of the x86_SSE test failures at -O0; drop them now that the fix is in.
SumATA2 relied on non-static data member initializers with the implicit default constructor. At -O0 on x86 (gcc on Ubuntu, Apple clang on the Mac Intel runner), those initializers are not applied, so the sum_AT and sum_A2 members kept stale stack contents and the results were machine-dependent garbage (e.g. -8e-34 instead of -0.628, differing per runner while the identical -O2 builds passed). Give SumATA2 an explicit user-defined default constructor that initializes both members from Context::vzero(), and move SumError's sum member into its constructor's member-initializer list. This is the same pattern the CI evidence showed working under -O0 (the SumError error-path results matched the reference bit-for-bit on the failing runners).
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Adds unit tests for the four ScaleInvariantMatrixMatch kernel functions (compute_scale and compute_error, plain and weighted), with the Default path as the source of truth.
Also removes the unused "Common/Cpp/Exceptions.h" include from Kernels_ScaleInvariantMatrixMatch.cpp (separate commit).
The NEON (arm64) implementation will be a follow-up PR stacked on this one.